-
Notifications
You must be signed in to change notification settings - Fork 276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ci] disallow failures in fmt and clippy #178
Conversation
@alexcrichton What do you think about this? I guess I'm OK with this, since I agree that there is otherwise little point of having these things in CI if we aren't going to actually look at them. If this breaks the build too frequently, then I'd probably suggest just removing them and say that we gave it the good ol' college try. |
I'd personally still be wary of doing this. I fear that this will increase the threshold for contribution relatively significantly. Clippy is likely to break multiple times a week, meaning the CI would probably rarely be green. Additionally rustfmt is still (afaik) somewhat rapidly change, so it as well is likely to break throughout each week. I agree that I don't personally see much use in running CI builders that are allowed to fail, but we've got the capacity as they're fast enough so I wouldn't block them. All in all I would prefer to not merge this until we get to the point where we could leave the repo alone for like a week, have a new contributor send a PR, and the CI is still green. |
3bc7978
to
7584716
Compare
I've moved the fmt and clippy fixes to a different PR and left here the changes required to make the clippy and fmt builds always fail. On a C++ project we do the following with We could do the same for clippy. That is have an @oli-obk Is it possible to enable/disable clippy lints globally in the clippy.toml file? |
@gnzlbg |
The issue is: rust-lang/rust-clippy#1313 |
@gnzlbg that could work yeah for the actual lints themselves but I don't think that'd solve the "rustdoc is changing" problem or the "sometimes clippy just doesn't build" problem? |
@alexcrichton yes for that we need to wait till clippy-nightly and rustfmt-nightly are distributed with nightly rust via rustup (that is, so that we don't have to build them to use them). I hope this will happen sooner rather than later. |
Indeed yeah, at that point I'd be much more comfortable about making this change! |
Blocked by rust-lang/rustup#1285 , closed until that's fixed. |
Reviewers interpret the travis "green light" as "ok to merge" instead of "check if the allowed failures are green, and then merge". The proof is that basically every pull-request since rustfmt was enabled has broken the formatting. This pollutes unrelated pull-requests with formatting changes.
The downside of this is that when clippy and rustfmt fail to build, the PRs will appear "red". Nothing more, nothing less.
In particular, I want to emphasize that red pull-requests can still be merged. Just check that everything is green, and that the reason, e.g., formatting is broken is because rustfmt-nigthly doesn't build and not because the submitter forgot to run it.